Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add declarative smart answer helper methods #4527

Merged
merged 6 commits into from
Jun 4, 2020
Merged

Conversation

kevindew
Copy link
Member

Addresses #4510 - I've opened this up as a draft, as a proposal to resolve the issue

This adds text_for, govspeak_for and html_for methods intended
for the smart answer flow methods as a way for users to define content
for a smart answer. These are intended to replace the
render_content_for helper that has logic to determine the content
type.

This is set-up to be backwards compatible with the render_content_for
method so that both can co-exist for a period of time to allow
migration.

In doing this I also simplified a few things. We don't make any use of
options when calling content_for so these methods don't need to
support an option. All our usages of these methods use a block to add
content so I've set it up to raise an error.

@kevindew kevindew requested a review from theseanything May 19, 2020 10:25
@bevanloon bevanloon temporarily deployed to smart-answer-explanator-4msycg May 19, 2020 10:25 Inactive
@theseanything
Copy link
Contributor

Awesome 🎉 Thanks @kevindew - it does read rather nicely! I'd be happy with this change - what you've got in draft looks good. Think we just need some docs and tests for the new syntax. Then another PR or two to roll out the changes to the other smart answers?

Originally, I was worried about someone using the wrong helper method for content that can on be certain format (e.g. govspeak_for :title). Not sure how we'd mitigate that, think we are reliant on people noticing manually.

@kevindew kevindew force-pushed the explanatory-helpers branch from 97e1145 to 64e1d23 Compare May 20, 2020 10:32
@bevanloon bevanloon temporarily deployed to smart-answer-explanator-4msycg May 20, 2020 10:32 Inactive
@kevindew kevindew force-pushed the explanatory-helpers branch from 64e1d23 to cbd853a Compare May 21, 2020 18:53
@bevanloon bevanloon temporarily deployed to smart-answer-explanator-4msycg May 21, 2020 18:54 Inactive
@kevindew
Copy link
Member Author

Thanks @theseanything I've made some updates for this now. I've updated the documentation, added tests for the functionality and I've also enabled the ability for smart answer flow files to have an extension of .erb so to not require the .govspeak part of the extension.

Regarding your concern of someone using govspeak_for :title style, I've iterated on the format definitions you introduced to set up a check for times where people use govspeak_for or html_for. Now if you do govspeak_for :title an exception will be raised.

Previously I wasn't too concerned about that kind of mis-use, as it seems just one of many ways someone could make a mistake but it does seem like doing so could create a rather transparent issue. I've also realised that having this in will help with the migration over to this approach since it will break rather indignantly compared to succeeding quietly 😄

I think this is now ready for review. So I'll mark this so and open a draft of the big migration of answers 😬 .

@kevindew kevindew marked this pull request as ready for review May 21, 2020 19:02
Copy link
Contributor

@theseanything theseanything left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kevindew! - I've added a few comments.

doc/smart-answers/erb-templates.md Outdated Show resolved Hide resolved
doc/smart-answers/erb-templates.md Outdated Show resolved Hide resolved
doc/smart-answers/erb-templates.md Outdated Show resolved Hide resolved
doc/smart-answers/erb-templates/landing-page-template.md Outdated Show resolved Hide resolved
doc/smart-answers/erb-templates/outcome-templates.md Outdated Show resolved Hide resolved
lib/smart_answer/erb_renderer.rb Outdated Show resolved Hide resolved
lib/smart_answer/erb_renderer/format_capture_helper.rb Outdated Show resolved Hide resolved
lib/smart_answer/erb_renderer/format_capture_helper.rb Outdated Show resolved Hide resolved
doc/smart-answers/erb-templates.md Show resolved Hide resolved
@kevindew kevindew force-pushed the explanatory-helpers branch from cbd853a to 871169a Compare May 27, 2020 13:17
@kevindew
Copy link
Member Author

Thanks for the review @theseanything 👍 Something else has just come up so I've left this for now with just comments on your doc comments. Will return later.

I've also removed the applying of this to the coronavirus example since it'll need rebasing and I think it's already served it's purpose as a demo.

@theseanything
Copy link
Contributor

@kevindew np!! No rush! - Feel free to hit the re-request review button 🛎️ when your ready!

@kevindew kevindew force-pushed the explanatory-helpers branch 2 times, most recently from 95a797b to 0215636 Compare May 29, 2020 09:28
@kevindew
Copy link
Member Author

kevindew commented Jun 2, 2020

@theseanything this is good for another review - thanks 🐱

kevindew added 6 commits June 4, 2020 10:49
This adds `text_for`, `govspeak_for` and `html_for` methods intended
for the smart answer flow methods as a way for users to define content
for a smart answer. These are intended to replace the
`render_content_for` helper that has logic to determine the content
type.

This is set-up to be backwards compatible with the render_content_for
method so that both can co-exist for a period of time to allow
migration.

In doing this I also simplified a few things. We don't make any use of
options when calling `content_for` so these methods don't need to
support an option. All our usages of these methods use a block to add
content so I've set it up to raise an error.

I also removed the need to remove excess new lines after govspeak is
rendered. Since we already have HTML at that point I'm not sure what
value that adds.
This change allows template files to lack the .govspeak part of their
filename. For example lib/smart_answer_flows/business-coronavirus-support-finder/business_coronavirus_support_finder.govspeak.erb
can be renamed lib/smart_answer_flows/business-coronavirus-support-finder/business_coronavirus_support_finder.erb

This is done to reflect that these files no longer only contain govspeak
(arguably they never did since they were a mix of govspeak and plain
text) and thus the govspeak extension adds confusion. Since they are
mixed media it makes more sense for them to just have a ".erb"
extension.

Rendering and looking up of templates are changed to be done by the
template lookup system of action view rather than the previous manual
path manipulation. The lookup work only seems to be used as a means to
output extra help text in views, which seems potentially unnecessary. I
didn't want to remove this functionality here though as I don't know if
someone does use it.

I did move the erb_template_path to be private since nothing external
seems to use it.
These tests were all tested functionality defined outside this test.
This adds in a check when specifying HTML in a smart answer (via
govspeak_for or html_for) that it is not for a field that is text only.
This is put in to address a concern that it could be rather easy for
someone to specify `govspeak_for(:title) { "my govspeak" }` and this
produce an unexpected output that no-one notices.

As part of this I removed the govspeak option in DEFAULT_FORMATS as I
didn't think it affected the outcome of any method calls.
This updates the documentation to remove references to .govspeak.erb
files and to make suggestions of which helper method (text_for,
govspeak_for, html_for) in each scenario.

Along the way I noticed that the label field may not be working (issue
prior to these changes) so I left a note about that so the next person
who wants it can investigate.

I removed the information on error_* as I believe that is mostly set on
a custom basis so doesn't need to be included in the general
documentation.
This uses the new text_for & govspeak_for helpers and drops the
.govspeak extension from the filename.
@kevindew kevindew force-pushed the explanatory-helpers branch from 0215636 to 28fb339 Compare June 4, 2020 09:49
@kevindew kevindew merged commit dd22e18 into master Jun 4, 2020
@kevindew kevindew deleted the explanatory-helpers branch June 4, 2020 11:04
kevindew added a commit that referenced this pull request Jul 14, 2020
Since #4527 was merged we
no longer use the render_content_for method in smart answers and can now
safely remove it.
kevindew added a commit that referenced this pull request Jul 14, 2020
Since #4527 was merged we
no longer use the render_content_for method in smart answers and can now
safely remove it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants